chore: improve voice call logs #37861
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis PR refactors logging throughout the media-calls package and related services. It removes numerous debug log statements to reduce verbosity, adds selective info and warning logs for key state transitions, updates several hangup method return types from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
ee/packages/media-calls/src/server/BroadcastAgent.ts (1)
30-32: Consider removing this debug log for consistency.This is the only lifecycle method in this file that includes logging. Given that the PR removes debug logs from other lifecycle methods to reduce verbosity, this log seems inconsistent. Additionally, logging the entire
callobject at debug level could expose sensitive information (user IDs, phone numbers) and may be verbose.If detailed logging is needed specifically for call creation, consider whether it should be moved elsewhere or if an info-level log would be more appropriate per the PR's stated goals.
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
347-353: Empty catch block silently swallows errors.The
.catch(() => null)silently discards any errors fromsetStableById. While this may be intentional to avoid blocking the flow, it could hide database issues. Consider logging at debug level for operational visibility.void MediaCallNegotiations.setStableById(signal.negotiationId) .then((result) => { if (result.modifiedCount) { logger.info({ msg: 'Negotiation is stable', callId: signal.callId, role: this.role }); } }) - .catch(() => null); + .catch((err) => { + logger.debug({ msg: 'Failed to set negotiation stable', negotiationId: signal.negotiationId, err }); + });ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (1)
433-436: Consider consolidating duplicate case branches.Both
DECLINEDandUNWANTEDreturn'rejected'. These can be combined for clarity.switch (errorCode) { case SipErrorCodes.DECLINED: - return 'rejected'; case SipErrorCodes.UNWANTED: return 'rejected'; case SipErrorCodes.NOT_ACCEPTABLE_HERE: return 'service-error'; }ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (1)
266-271: Include callId in the REFER failure error log for consistency.The error log for REFER failure is missing
callId, which was present in similar error logs (e.g., line 139). Including it would improve traceability.- logger.error({ msg: 'REFER failed', method: 'IncomingSipCall.processTransferredCall', err }); + logger.error({ msg: 'REFER failed', method: 'IncomingSipCall.processTransferredCall', err, callId: call._id });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/meteor/server/services/media-call/service.ts(0 hunks)ee/packages/media-calls/src/internal/InternalCallProvider.ts(1 hunks)ee/packages/media-calls/src/internal/SignalProcessor.ts(2 hunks)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts(5 hunks)ee/packages/media-calls/src/internal/agents/UserActorAgent.ts(1 hunks)ee/packages/media-calls/src/server/BroadcastAgent.ts(1 hunks)ee/packages/media-calls/src/server/CallDirector.ts(11 hunks)ee/packages/media-calls/src/server/CastDirector.ts(2 hunks)ee/packages/media-calls/src/server/MediaCallServer.ts(3 hunks)ee/packages/media-calls/src/sip/errorCodes.ts(1 hunks)ee/packages/media-calls/src/sip/providers/BaseSipCall.ts(2 hunks)ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts(8 hunks)ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts(10 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/server/services/media-call/service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/media-calls/src/sip/errorCodes.tsee/packages/media-calls/src/sip/providers/BaseSipCall.tsee/packages/media-calls/src/internal/InternalCallProvider.tsee/packages/media-calls/src/internal/agents/CallSignalProcessor.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsee/packages/media-calls/src/server/MediaCallServer.tsee/packages/media-calls/src/server/CastDirector.tsee/packages/media-calls/src/sip/providers/OutgoingSipCall.tsee/packages/media-calls/src/sip/providers/IncomingSipCall.tsee/packages/media-calls/src/server/BroadcastAgent.tsee/packages/media-calls/src/internal/SignalProcessor.tsee/packages/media-calls/src/server/CallDirector.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
📚 Learning: 2025-10-28T19:39:58.182Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37328
File: packages/ui-voip/src/v2/useTonePlayer.ts:29-30
Timestamp: 2025-10-28T19:39:58.182Z
Learning: In packages/ui-voip/src/v2/useTonePlayer.ts, the DTMF tones generated by TonePlayer are for local auditory feedback to the user only. The actual DTMF signals are transmitted separately through the WebRTC channel. Therefore, filter configurations should prioritize user comfort and audio quality over technical DTMF frequency accuracy.
Applied to files:
ee/packages/media-calls/src/sip/providers/BaseSipCall.tsee/packages/media-calls/src/internal/agents/UserActorAgent.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
ee/packages/media-calls/src/internal/InternalCallProvider.tsee/packages/media-calls/src/internal/agents/UserActorAgent.tsee/packages/media-calls/src/server/MediaCallServer.tsee/packages/media-calls/src/server/CastDirector.tsee/packages/media-calls/src/sip/providers/OutgoingSipCall.tsee/packages/media-calls/src/sip/providers/IncomingSipCall.tsee/packages/media-calls/src/server/BroadcastAgent.tsee/packages/media-calls/src/server/CallDirector.ts
🧬 Code graph analysis (11)
ee/packages/media-calls/src/sip/providers/BaseSipCall.ts (1)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
ee/packages/media-calls/src/internal/InternalCallProvider.ts (1)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (3)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)ee/packages/media-calls/src/server/stripSensitiveData.ts (1)
stripSensitiveDataFromSignal(16-24)packages/models/src/index.ts (1)
MediaCallNegotiations(188-188)
ee/packages/media-calls/src/internal/agents/UserActorAgent.ts (1)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
ee/packages/media-calls/src/server/MediaCallServer.ts (2)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)ee/packages/media-calls/src/server/stripSensitiveData.ts (1)
stripSensitiveDataFromSignal(16-24)
ee/packages/media-calls/src/server/CastDirector.ts (1)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (4)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)packages/media-signaling/src/definition/call/IClientMediaCall.ts (1)
CallHangupReason(30-43)ee/packages/media-calls/src/server/CallDirector.ts (1)
mediaCallDirector(434-434)ee/packages/media-calls/src/sip/errorCodes.ts (1)
SipErrorCodes(1-24)
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (2)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)ee/packages/media-calls/src/server/CallDirector.ts (1)
mediaCallDirector(434-434)
ee/packages/media-calls/src/server/BroadcastAgent.ts (4)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (1)
callId(32-34)packages/media-signaling/src/lib/Call.ts (1)
callId(54-56)ee/packages/media-calls/src/base/BaseCallProvider.ts (1)
callId(7-9)
ee/packages/media-calls/src/internal/SignalProcessor.ts (2)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)ee/packages/media-calls/src/server/stripSensitiveData.ts (1)
stripSensitiveDataFromSignal(16-24)
ee/packages/media-calls/src/server/CallDirector.ts (2)
ee/packages/media-calls/src/logger.ts (1)
logger(3-3)packages/core-typings/src/mediaCalls/IMediaCall.ts (2)
IMediaCall(35-68)ServerActor(17-20)
🔇 Additional comments (27)
ee/packages/media-calls/src/server/BroadcastAgent.ts (1)
34-34: LGTM!Good use of the underscore prefix to indicate the
negotiationIdparameter is intentionally unused.ee/packages/media-calls/src/internal/InternalCallProvider.ts (1)
49-53: LGTM! Excellent addition of transfer logging.The info-level log provides clear visibility into call transfer operations and includes both call IDs for correlation, which will be valuable for debugging and monitoring.
ee/packages/media-calls/src/server/CallDirector.ts (11)
42-49: LGTM! Appropriate info log for call activation.The log is correctly placed after verifying the state change succeeded (
modifiedCountcheck) and provides useful visibility into the call lifecycle.
65-83: LGTM! Well-structured logging for call acceptance flow.Both info logs are appropriately guarded by
modifiedCountchecks and include relevant identifiers (callId, negotiationId) for correlation. This provides good visibility into the acceptance flow without creating log noise from no-op operations.
117-119: LGTM! Clear logging for negotiation lifecycle.The info log provides good context with the negotiation ID and whether an offer was included, which will be helpful for debugging WebRTC negotiation flows.
158-166: LGTM! Dynamic logging based on SDP type.The log appropriately reflects whether an offer or answer was saved and is correctly guarded by the
modifiedCountcheck. The inclusion of both callId and negotiationId aids in tracing the negotiation flow.
229-231: LGTM! Essential log for call creation.This info log marks a key lifecycle event and is placed right after the call is successfully created and retrieved from the database.
247-254: LGTM! Appropriate logging for call transfers.The info log correctly marks when a call has been flagged as transferred, providing visibility into an important state transition. The log is properly guarded by the
modifiedCountcheck.
257-265: LGTM! Correct boolean return logic.The method correctly returns
falsewhen the call doesn't exist or wasn't transferred, and propagates the boolean result fromhangupDetachedCallotherwise. This provides useful feedback to callers about whether a modification occurred.
379-385: LGTM! Critical state transition properly logged.The info log marks when a call ends and includes the reason, which directly supports the PR objective of "improved identification and logging of the reason why a call ended." The log is appropriately guarded by the
endedflag.
388-404: LGTM! Correct boolean return propagation.The method properly tracks and returns the modification status from
hangupCallById. The early return optimization (line 394-396) is appropriate, and the design correctly treats agent notification failures as non-fatal (they don't affect the modification status).
406-431: LGTM! Robust boolean return logic with proper error handling.The modification tracking is correctly implemented:
- Returns
falseearly if the hangup didn't modify anything (line 414)- Returns
trueif the call was ended, even if agent notification fails (line 426)- In the error catch, returns the current
modifiedstate, which is appropriate since the call may have been ended before the error occurred (line 429)This design correctly prioritizes the core operation (ending the call) over the secondary operation (notifying agents).
35-36: All callers properly handle the return type change.The return type change from
Promise<void>toPromise<boolean>is handled correctly. All six callers ofhangupByServeracross OutgoingSipCall.ts and IncomingSipCall.ts explicitly prefix the call withvoid, which is valid TypeScript and acknowledges the return value while intentionally discarding it. This is an appropriate pattern for fire-and-forget async operations.ee/packages/media-calls/src/sip/providers/BaseSipCall.ts (1)
7-7: LGTM!The logger import and debug log addition for
sendDTMFare appropriate for tracing purposes. The log statement is minimal and doesn't expose sensitive data.Also applies to: 48-48
ee/packages/media-calls/src/sip/errorCodes.ts (1)
22-23: LGTM!The addition of
UNWANTED: 607is appropriate. SIP 607 is a valid RFC-defined response code for unwanted calls (RFC 8197), and it's correctly placed in the 6xx response class alongsideDECLINED.ee/packages/media-calls/src/internal/agents/CallSignalProcessor.ts (2)
83-90: Good selective logging with proper data sanitization.The conditional logging that excludes
local-statesignals reduces noise while still capturing meaningful events. UsingstripSensitiveDataFromSignalensures SDP and other sensitive data is properly sanitized before logging.
320-326: LGTM!The info-level log for channel activation is a valuable addition for tracking key state transitions, aligning with the PR objective of marking key call state changes.
ee/packages/media-calls/src/sip/providers/OutgoingSipCall.ts (2)
427-442: LGTM!The new
getHangupReasonForSipErrorCodehelper provides clean mapping of SIP error codes to semantic hangup reasons. The handling ofDECLINED(603) andUNWANTED(607) as 'rejected', andNOT_ACCEPTABLE_HERE(488) as 'service-error' is appropriate.
191-197: Good differentiation between natural hangup reasons and actual errors.The conditional logging that distinguishes between natural hangup reasons (like 'rejected') and actual errors improves log clarity. Using error level for unexpected failures and info level for expected SIP responses is appropriate.
ee/packages/media-calls/src/internal/SignalProcessor.ts (2)
112-114: LGTM!The debug log for
processRegisterSignalproperly sanitizes the signal data before logging, maintaining security while providing useful tracing information.
142-144: LGTM!The info-level log for client refresh detection is a valuable addition for tracking reconnection scenarios. This helps identify when a client refreshes mid-call, which is useful for debugging call state issues.
ee/packages/media-calls/src/server/MediaCallServer.ts (2)
55-56: LGTM!Logging the signal with sensitive data stripped via
stripSensitiveDataFromSignalensures SDP and other sensitive information is not exposed in logs while still providing useful debugging context.
154-156: LGTM!Elevating the "Invalid call requester" message from debug to warn level is appropriate. This condition indicates a potentially malformed or malicious request that operators should be aware of.
ee/packages/media-calls/src/sip/providers/IncomingSipCall.ts (3)
139-141: LGTM!Adding
callIdto the error log and using'signaling-error'as the hangup reason provides better context for debugging dialog creation failures. The error categorization is more accurate.
203-207: LGTM!The info-level log when a SIP call is canceled by the caller is a valuable addition for tracking call lifecycle events. This aligns with the PR objective of marking key changes in call state.
275-276: LGTM!Including
hangupReasonin theprocessEndedCalldebug log provides valuable context for understanding why a call ended, improving post-call analysis capabilities.ee/packages/media-calls/src/server/CastDirector.ts (1)
74-76: Null handling is already correct throughout the call chain.All callers of
getContactForActorproperly check for null before using the result. The return type signature already permitsnull, and both MediaCallServer.ts and the SIP providers handle the null case with appropriate error logging and exception throwing.Likely an incorrect or invalid review comment.
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 17:41", "12/18 18:57 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 24 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 18 Dec 2025 18:57:57 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37861 +/- ##
===========================================
+ Coverage 67.75% 67.83% +0.08%
===========================================
Files 3463 3463
Lines 113702 113721 +19
Branches 20901 20962 +61
===========================================
+ Hits 77034 77145 +111
+ Misses 34501 34401 -100
- Partials 2167 2175 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
pierre-lehnen-rc
added
the
stat: QA assured
tassoevan
added
stat: QA assured
dionisio-bot
bot
removed
the
stat: QA assured
dionisio-bot
bot
added
stat: QA assured
dionisio-bot
bot
added
stat: ready to merge
dionisio-bot
bot
added
stat: QA assured
dionisio-bot
bot
added
stat: QA assured
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
debuglog entries that were causing the same data to be logged multiple timesinfolog entries to identify key changes to the call's state.Issue(s)
VGA-117
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.